Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

app/apptesting: Reduce memory allocations in KeeperTestHelper.Setup() #3568

Merged
merged 2 commits into from
Dec 3, 2022
Merged

app/apptesting: Reduce memory allocations in KeeperTestHelper.Setup() #3568

merged 2 commits into from
Dec 3, 2022

Conversation

pysel
Copy link
Member

@pysel pysel commented Nov 29, 2022 •
edited by ValarDragon
Loading

Component of: #2148

What is the purpose of the change

Memprofile KeeperTestHelper.Setup() and reduce memory allocs

Using Benchmark added in #2143

Steps Taken

  • Cache encoding config in app/encoding.go
Initial benchmark:
   BenchmarkSetup-8             514           2258866 ns/op         1364230 B/op      17732 allocs/op
Benchmark after caching encoding config: 
   BenchmarkSetup-8             534           1902947 ns/op         1077572 B/op      14312 allocs/op

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? no
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? no
  • How is the feature or change documented? no

@github-actions github-actions bot added the C:app-wiring Changes to the app folder label Nov 29, 2022
@ValarDragon
Copy link
Member

ValarDragon commented Nov 30, 2022 •
edited
Loading

Thanks for picking this up! Apologies though, having a hard time understanding the benchmark pasted.

Is that the output for a particular function?

We can see overall impact by doing
go test -benchmem -run=^$ -bench ^BenchmarkSetup$ github.com/osmosis-labs/osmosis/v13/app/apptesting before and after. (If you use vscode, just click run benchmark above the function)

You'd get something like:

BenchmarkSetup-16    	     398	   2977221 ns/op	 1356578 B/op	   17590 allocs/op

And we should see both time & allocs/op go down

@pysel
Copy link
Member Author

pysel commented Nov 30, 2022

oh, it is from go tool pprof. Yes, you are right, it shows memory allocations for a particular function. I will rerun it the way you specified and add it here :)

@ValarDragon ValarDragon added the V:state/compatible/backport State machine compatible PR, should be backported label Nov 30, 2022
@github-actions github-actions bot added C:simulator Edits simulator or simulations C:x/pool-incentives labels Dec 3, 2022
@pysel pysel force-pushed the tests_CI_save branch 2 times, most recently from 361e3ce to 55f243f Compare December 3, 2022 04:46
@github-actions github-actions bot removed C:x/pool-incentives C:simulator Edits simulator or simulations labels Dec 3, 2022
@ValarDragon
Copy link
Member

This LGTM, nice! Other followups involve work in other repos, so I recommend marking this ready for review, and we get this merged?

@ValarDragon ValarDragon added the A:backport/v13.x backport patches to v13.x branch label Dec 3, 2022
@ValarDragon ValarDragon marked this pull request as ready for review December 3, 2022 15:40
@ValarDragon ValarDragon merged commit 4751054 into osmosis-labs:main Dec 3, 2022
@mergify mergify bot mentioned this pull request Dec 3, 2022
Merged
mergify bot pushed a commit that referenced this pull request Dec 3, 2022
@pysel @mergify
cache encoding config (#3568)
45e9bd9 
Co-authored-by: Ruslan Akhtariev <[email protected]>
(cherry picked from commit 4751054)
ValarDragon pushed a commit that referenced this pull request Dec 3, 2022
@mergify @pysel
cache encoding config (#3568) (#3610)
c0224eb 
Co-authored-by: Ruslan Akhtariev <[email protected]>
(cherry picked from commit 4751054)

Co-authored-by: Ruslan Akhtariev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ValarDragon ValarDragon ValarDragon approved these changes

Assignees
No one assigned
Labels
A:backport/v13.x backport patches to v13.x branch C:app-wiring Changes to the app folder V:state/compatible/backport State machine compatible PR, should be backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pysel @ValarDragon